-
-
Notifications
You must be signed in to change notification settings - Fork 62
build: support clang tidy #1118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0ff58c3
to
8279d8b
Compare
mhofstetter
reviewed
Feb 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks a lot for this Jarno! Left some nits inline.
- What about linting and fixing the files in
starter
too? There seem to be a lot of issues left. - Going over the code base with your changes applied, i discovered the following additional changes that might be worth to change. Maybe you want to apply them.
diff --git a/cilium/accesslog.h b/cilium/accesslog.h
index 2dd4af8f..b744720a 100644
--- a/cilium/accesslog.h
+++ b/cilium/accesslog.h
@@ -53,7 +53,7 @@ public:
void addRejected(absl::string_view key, absl::string_view value);
void addMissing(absl::string_view key, absl::string_view value);
- ::cilium::LogEntry entry_{};
+ ::cilium::LogEntry entry_;
bool request_logged_ = false;
};
diff --git a/cilium/bpf_metadata.h b/cilium/bpf_metadata.h
index 277359af..42dc36b8 100644
--- a/cilium/bpf_metadata.h
+++ b/cilium/bpf_metadata.h
@@ -149,10 +149,10 @@ public:
Network::Address::InstanceConstSharedPtr ipv6_source_address_;
bool enforce_policy_on_l7lb_;
- std::shared_ptr<const Cilium::NetworkPolicyMap> npmap_{};
- Cilium::CtMapSharedPtr ct_maps_{};
- Cilium::IPCacheSharedPtr ipcache_{};
- std::shared_ptr<const Cilium::PolicyHostMap> hosts_{};
+ std::shared_ptr<const Cilium::NetworkPolicyMap> npmap_;
+ Cilium::CtMapSharedPtr ct_maps_;
+ Cilium::IPCacheSharedPtr ipcache_;
+ std::shared_ptr<const Cilium::PolicyHostMap> hosts_;
private:
uint32_t resolveSourceIdentity(const PolicyInstance& policy, const Network::Address::Ip* sip,
diff --git a/cilium/network_filter.h b/cilium/network_filter.h
index 0add9f96..a27003dd 100644
--- a/cilium/network_filter.h
+++ b/cilium/network_filter.h
@@ -66,10 +66,10 @@ private:
Network::ReadFilterCallbacks* callbacks_ = nullptr;
uint32_t remote_id_ = 0;
uint16_t destination_port_ = 0;
- std::string l7proto_{};
+ std::string l7proto_;
bool should_buffer_ = false;
Buffer::OwnedImpl buffer_; // Buffer for initial connection data
- Cilium::GoFilter::InstancePtr go_parser_{};
+ Cilium::GoFilter::InstancePtr go_parser_;
Cilium::AccessLog::Entry log_entry_{};
};
diff --git a/cilium/network_policy.cc b/cilium/network_policy.cc
index a4c7e095..0a6e0739 100644
--- a/cilium/network_policy.cc
+++ b/cilium/network_policy.cc
@@ -619,7 +619,7 @@ public:
// Use std::less<> to allow heterogeneous lookups (with string_view).
std::set<std::string, std::less<>> allowed_snis_; // All SNIs allowed if empty.
std::vector<HttpNetworkPolicyRule> http_rules_; // Allowed if empty, but remote is checked first.
- std::string l7_proto_{};
+ std::string l7_proto_;
std::vector<L7NetworkPolicyRule> l7_allow_rules_;
std::vector<L7NetworkPolicyRule> l7_deny_rules_;
};
@@ -1077,7 +1077,7 @@ public:
}
PolicyMap rules_;
- RulesList wildcard_rules_{};
+ RulesList wildcard_rules_;
};
// Construction is single-threaded, but all other use is from multiple worker threads using const
@@ -1363,7 +1363,7 @@ void NetworkPolicyMap::runAfterAllThreads(std::function<void()> cb) const {
//
// For now we rely on the implementation dependent fact that the reference returned by
// context_.threadLocal() actually is a ThreadLocal::Instance reference, where
- // runOnAllWorkerThreads() is exposed. Withtout this cast we'd need to use a dummy thread local
+ // runOnAllWorkerThreads() is exposed. Without this cast we'd need to use a dummy thread local
// variable that would take a thread local slot for no other purpose than to avoid this type cast.
dynamic_cast<ThreadLocal::Instance&>(context_.threadLocal()).runOnAllWorkerThreads([]() {}, cb);
}
diff --git a/cilium/network_policy.h b/cilium/network_policy.h
index a19405bc..6c1a2baa 100644
--- a/cilium/network_policy.h
+++ b/cilium/network_policy.h
@@ -129,8 +129,8 @@ public:
: ipv4_(ipv4), ipv6_(ipv6){};
IPAddressPair(const cilium::NetworkPolicy& proto);
- Network::Address::InstanceConstSharedPtr ipv4_{};
- Network::Address::InstanceConstSharedPtr ipv6_{};
+ Network::Address::InstanceConstSharedPtr ipv4_;
+ Network::Address::InstanceConstSharedPtr ipv6_;
};
class PolicyInstance {
@@ -287,7 +287,7 @@ private:
// written, and emits CPU instructions to also make the CPU out-of-order-execution logic to not
// reorder any write operations to happen after the pointer itself is written. This guarantees
// that the map is not modified after the point when the worker threads can observe the new
- // pointer value, i.e., the map is actaully immutable (const) from that point forward.
+ // pointer value, i.e., the map is actually immutable (const) from that point forward.
// - when the pointer is read (by a worker thread) 'std::memory_order_acquire' in the load
// operation informs the compiler to emit CPU instructions to make the CPU
// out-of-order-execution logic to not reorder any reads from the new map to happen before the
diff --git a/cilium/secret_watcher.h b/cilium/secret_watcher.h
index a0811ee8..474251b8 100644
--- a/cilium/secret_watcher.h
+++ b/cilium/secret_watcher.h
@@ -81,7 +81,7 @@ public:
private:
Ssl::ServerContextConfigPtr server_config_;
std::vector<std::string> server_names_;
- Ssl::ServerContextSharedPtr server_context_ ABSL_GUARDED_BY(ssl_context_mutex_){};
+ Ssl::ServerContextSharedPtr server_context_ ABSL_GUARDED_BY(ssl_context_mutex_);
};
using DownstreamTLSContextPtr = std::unique_ptr<DownstreamTLSContext>;
@@ -98,7 +98,7 @@ public:
private:
Ssl::ClientContextConfigPtr client_config_;
- Ssl::ClientContextSharedPtr client_context_ ABSL_GUARDED_BY(ssl_context_mutex_){};
+ Ssl::ClientContextSharedPtr client_context_ ABSL_GUARDED_BY(ssl_context_mutex_);
};
using UpstreamTLSContextPtr = std::unique_ptr<UpstreamTLSContext>;
diff --git a/cilium/websocket_codec.h b/cilium/websocket_codec.h
index 67d3f3ba..817940d5 100644
--- a/cilium/websocket_codec.h
+++ b/cilium/websocket_codec.h
@@ -63,7 +63,7 @@ private:
Codec& parent_;
bool end_stream_{false};
- Buffer::OwnedImpl encoded_{}; // Buffer for encoded websocket frames
+ Buffer::OwnedImpl encoded_; // Buffer for encoded websocket frames
};
class Decoder : Logger::Loggable<Logger::Id::filter> {
@@ -79,8 +79,8 @@ private:
Codec& parent_;
bool end_stream_{false};
- Buffer::OwnedImpl buffer_{}; // Buffer for partial websocket frames
- Buffer::OwnedImpl decoded_{}; // Buffer for decoded websocket frames
+ Buffer::OwnedImpl buffer_; // Buffer for partial websocket frames
+ Buffer::OwnedImpl decoded_; // Buffer for decoded websocket frames
bool unmasking_{false};
uint8_t mask_[4];
@@ -124,7 +124,7 @@ private:
uint64_t ping_count_{0};
Event::TimerPtr handshake_timer_{nullptr};
- Buffer::OwnedImpl handshake_buffer_{};
+ Buffer::OwnedImpl handshake_buffer_;
bool accepted_{false};
};
using CodecPtr = std::unique_ptr<Codec>;
diff --git a/tests/cilium_tcp_integration_test.cc b/tests/cilium_tcp_integration_test.cc
index 24ca3817..1f7f0775 100644
--- a/tests/cilium_tcp_integration_test.cc
+++ b/tests/cilium_tcp_integration_test.cc
@@ -279,7 +279,7 @@ TEST_P(CiliumTcpProxyIntegrationTest, CiliumTcpProxyUpstreamFlushEnvoyExit) {
ASSERT_TRUE(fake_upstream_connection->close());
ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
- // Success criteria is that no ASSERTs fire and there are no leaks.
+ // Success criteria is that no ASSERT's fire and there are no leaks.
}
//
diff --git a/tests/cilium_tls_tcp_integration_test.cc b/tests/cilium_tls_tcp_integration_test.cc
index 84052f13..7e7e58db 100644
--- a/tests/cilium_tls_tcp_integration_test.cc
+++ b/tests/cilium_tls_tcp_integration_test.cc
@@ -500,7 +500,7 @@ TEST_P(CiliumTLSProxyIntegrationTest, CiliumTLSProxyUpstreamFlushEnvoyExit) {
ASSERT_TRUE(fake_upstream_connection->close());
ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
- // Success criteria is that no ASSERTs fire and there are no leaks.
+ // Success criteria is that no ASSERT's fire and there are no leaks.
}
//
diff --git a/tests/cilium_websocket_codec_integration_test.cc b/tests/cilium_websocket_codec_integration_test.cc
index b93f8702..adf51935 100644
--- a/tests/cilium_websocket_codec_integration_test.cc
+++ b/tests/cilium_websocket_codec_integration_test.cc
@@ -338,7 +338,7 @@ TEST_P(CiliumWebSocketIntegrationTest, CiliumWebSocketUpstreamFlushEnvoyExit) {
ASSERT_TRUE(fake_upstream_connection->close());
ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
- // Success criteria is that no ASSERTs fire and there are no leaks.
+ // Success criteria is that no ASSERT's fire and there are no leaks.
}
} // namespace Envoy
diff --git a/tests/cilium_websocket_encap_integration_test.cc b/tests/cilium_websocket_encap_integration_test.cc
index 126a857a..fe4ff67b 100644
--- a/tests/cilium_websocket_encap_integration_test.cc
+++ b/tests/cilium_websocket_encap_integration_test.cc
@@ -555,7 +555,7 @@ TEST_P(CiliumWebSocketIntegrationTest, CiliumWebSocketUpstreamFlushEnvoyExit) {
ASSERT_TRUE(fake_upstream_connection->close());
ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
- // Success criteria is that no ASSERTs fire and there are no leaks.
+ // Success criteria is that no ASSERT's fire and there are no leaks.
}
} // namespace Envoy
mhofstetter
approved these changes
Feb 11, 2025
Add the following clang packages to install_clang function: - clangd-17 - lldb-17 - clang-tools-17 - cland-tidy-17 Add make targets: - compile_commands.json: create the compile commands database needed for both clang-tidy and clangd. Include absl which otherwise was unreachable for clangd. - tidy: Use run-clang-tidy-17 to run clang-tidy in parallel. It is still very slow. - tidy-fix: Run clang tidy and fix errors. Use with caution, as the fixes do not always compile. Add query option --incompatible_merge_fixed_and_default_shell_env to .bazelrc so that query does not trigger re-download of envoy dependency. This same option is set on 'build' in 'envoy.bazelrc'. Add some include rules that seem necessary to .clangd and implement the same rules in .clang-tidy. Signed-off-by: Jarno Rajahalme <[email protected]>
Add ci-clang-tidy workflow, linting files changed from 'main'. Signed-off-by: Jarno Rajahalme <[email protected]>
Now that we have new targets 'tidy' and 'tidy-fix', rename the old clang-format targets 'check' and 'fix' as 'format' and 'format-fix', respectively. Signed-off-by: Jarno Rajahalme <[email protected]>
Signed-off-by: Jarno Rajahalme <[email protected]>
Signed-off-by: Jarno Rajahalme <[email protected]>
Use a separate local variable 'data_len' to make it clear that it does not need to be updated before 'input_len' is set. Signed-off-by: Jarno Rajahalme <[email protected]>
clang-tidy fix does not correctly handle renaming when removing trailing newlines from parameter names, so do this manually. Signed-off-by: Jarno Rajahalme <[email protected]>
Remove unnecessary includes, as well as unnecessary IWYU pragmas, while still keeping both clangd and clang-tidy happy. Signed-off-by: Jarno Rajahalme <[email protected]>
Replace typedefs with "using". Doing this as a separate step as clang-tidy fix will make errors doing this. Signed-off-by: Jarno Rajahalme <[email protected]>
Signed-off-by: Jarno Rajahalme <[email protected]>
Rename 'ops' as 'op_slice' and 'ops_' as 'ops'. This helps reduce confusion with member variables that have the '_' suffix. Signed-off-by: Jarno Rajahalme <[email protected]>
Signed-off-by: Jarno Rajahalme <[email protected]>
8279d8b
to
e44e3a3
Compare
Use CabelCase for type names to separate them better from function, method, and variable names. Keep "IP" capitalized when used for plural "IPs", or when referring to IP version: "IPv4", "IPv6". Otherwise align with Envoy convention to not capitalize the "p", e.g., "IpAddressPair". Treat "SNI" the same: "SniPattern", but "SNIs" where need be. Signed-off-by: Jarno Rajahalme <[email protected]>
e44e3a3
to
b26fda4
Compare
Clang tidy wants to include these, while IWYU tooling may want to remove this. Add pragma to stabilize this. Signed-off-by: Jarno Rajahalme <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add new clang-tidy make targets
tidy
andtidy-fix
. Rename existing clang-format targets fromcheck
andfix
toformat
andformat-fix
, respectively.Add a new GH workflow to run clang-tidy on changed files.
Bazel build options are passed into
gen_compilation_database.py
so that the analysis cache is not invalidated betweenmake tidy
andmake tests
targets, for example..clang-tidy
is copied from upstream Envoy, with addition ofmisc-include-cleaner
IgnoreHeaders
options adapted from.clangd
. Additionally, linting of local variables tolower_case
is added, which found out a few places where the "member variable suffix"_
was used on a local variable, in addition to somecamelCase
local variables that are now changed tolower_case
.By default
make tidy
will tidy up all sources intests
andcilium
directories, which takes a long time. Defining the variableTIDY_SOURCES
can be used to specify a subset, e.g.,:Please note that
make tidy-fix
is prone to producing duplicated includes and non-compiling code, so all edits made with it must be manually inspected.For the tidy targets to work, the host must have the package
clang-tidy-17
installed.